Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: add __eq__ on Table #1098

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aarcex3
Copy link

@aarcex3 aarcex3 commented Oct 14, 2024

Summary

Implemented the __eq__ method on the Table class to address the issue explained in #1075

Closes

#1075

PD: I would like to check the implementation first, that's why this is a DRAFT. Also I couldn't setup the project for testing, what I mean by that is, I tried using docker to run postgres and cockroach, but no luck. Am I missing something?

Also in which file in tests/table would it be best to add tests for this implementation?

Thanks

@sinisaos
Copy link
Member

@aarcex3 Thank you for your effort, but I think your solution is not correct. Sorry if I'm wrong. If you have this example

from piccolo.apps.user.tables import BaseUser
from piccolo_conf import DB

class Project(Table, db=DB):
    name = Varchar(length=100)
    owner = ForeignKey(BaseUser)

project = Project.objects(Project.owner).get(Project._meta.primary_key == 1).run_sync()
owner = BaseUser.objects().get(BaseUser._meta.primary_key == 1).run_sync()

print(project.owner == owner)
#  result is "piccolo_user"."id" = "piccolo_user"."id" and should be True

The solution from issue is better and the result is correct, but mypy complains. I think the solution should be something like this using getattr (Both results are correct and mypy is happy)

def __eq__(self, other) -> bool:
    return isinstance(other, Table) and getattr(
        self, self._meta.primary_key._meta.name, None
    ) == getattr(other, other._meta.primary_key._meta.name, None)

PD: I would like to check the implementation first, that's why this is a DRAFT. Also I couldn't setup the project for testing, what I mean by that is, I tried using docker to run postgres and cockroach, but no luck. Am I missing something?

I have Postgres and Cockroach installed on my local machine (for testing). I don't use Docker for testing so not much help from me. If you need help installing and using Cockroach locally, just ask I'll be glad to help.

Also in which file in tests/table would it be best to add tests for this implementation?

I would write the test here because it makes the most sense to me. Sorry for long comment.

@aarcex3
Copy link
Author

aarcex3 commented Oct 16, 2024

Hi @sinisaos, thank you so much for the feedback!!.
I will give it a try and let you know.

@dantownsend
Copy link
Member

Yeah, the solution that @sinisaos looks right.

When testing, I just run Postgres locally (i.e. not in a container). I use Postgres.app on the Mac, or just plain apt install postgresql on Ubuntu.

piccolo/table.py Outdated Show resolved Hide resolved
@sinisaos
Copy link
Member

@aarcex3 For CockroachDB you can also install locally (use the installation for your OS). After installation (this is for a Linux machine, it should be similar on a Mac, I don't know about Windows)

  1. Open a terminal and run CockroachDB (in single insecure node)
cockroach start-single-node --insecure
  1. Open another terminal and use this command to create the database
cockroach sql --insecure --execute="DROP DATABASE IF EXISTS piccolo CASCADE;CREATE DATABASE piccolo;"
  1. And finally you can start testing from piccolo directory
./scripts/test-cockroach.sh

@sinisaos
Copy link
Member

@aarcex3 Were you able to run the test on your local machine? Here is the branch with code changes and tests. All the code is formatted and ready to merge (if it's good enough Daniel will merge it) and feel free to use it for this PR. To make sure the formatting and linter are satisfied, just run ./scripts/format.sh and ./scripts/lint.sh from piccolo directory. Thanks again for your interest in Piccolo. Hope this helps.

@aarcex3 aarcex3 marked this pull request as ready for review October 17, 2024 00:52
@aarcex3
Copy link
Author

aarcex3 commented Oct 17, 2024

Hey @sinisaos, thank you so much for your help and guidance!!!. You didn't have to do all the work 😅 (I wanted to learn and try). But reading others people code is also useful 🤓.
Also, yes, I managed to run cockroach on my machine 🤩.

@sinisaos
Copy link
Member

You didn't have to do all the work 😅 (I wanted to learn and try)

@aarcex3 Sorry, you're right. I just wanted to help, because I know when you're contributing to a project for the first time it can be frustrating. I won't make the same mistake again.

I ran the CI workflow and everything passed. Now you have to wait for @dantownsend to review or merge. Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants